Fix fastdev divergence bug - #5420
Conversation
There was a problem hiding this comment.
Pull request overview
Separates Soroban fuzz targets from Rust test utilities to prevent fastdev builds from enabling production-host recording behavior.
Changes:
- Adds a dedicated
fuzz_targetsCargo feature. - Disables fuzz targets in fastdev builds.
- Retains fuzz smoke tests in normal test builds.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/rust/src/soroban_fuzz.rs |
Gates fuzz implementations on fuzz_targets. |
src/rust/Cargo.toml |
Defines the separate fuzz-target feature. |
src/Makefile.am |
Enables fuzz targets only for non-fastdev test builds. |
Suppressed comments (1)
src/rust/src/soroban_fuzz.rs:30
- The preceding comment is stale after splitting the features: these implementations are enabled by
fuzz_targets, nottestutils. Please update the comment accordingly.
#[cfg(feature = "fuzz_targets")]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Stub implementation when testutils feature is disabled | ||
| #[cfg(not(feature = "testutils"))] | ||
| #[cfg(not(feature = "fuzz_targets"))] |
| # | ||
| # One important caveat: the $(CARGO_FEATURE_FUZZ_TARGETS) feature should _not_ | ||
| # be passed to fastdev mode. It pulls in a separate copy of the soroban host | ||
| # with `testutils` enabled, which winds up up causing `testutils` to be enabled |
|
@graydon two things -
|
Oh, interesting! Hmm. This further complicates matters. That flag doesn't exist but re: point 2, we actually need to replace it, to pass fastdev -- for the build to be a unified build -- because fuzz instrumentation (or at least asan instrumentation, which the fuzzers usually want to have turned on) doesn't like separate .a files, it winds up with multiple asan runtimes linked in. Unfortunately if I do that, with this patch, the soroban fuzzers are disabled. Which is not what we want. Like the fuzz target binaries get built but they all just say "warning: target disabled" over and over while they loop. So .. sigh .. need to think more about this. |
Fastdev mode had unanticipated consequences:
soroban-fuzz-targetsdependency inrust_stellar_coreactually pulls in its own specific copy ofsoroban-env-host, which is the one it fuzzes. We can debate whether this is ideal (it's probably not, since it can fall behind the production host in master, but .. it's what it does currently).soroban-env-host/testutils, specifically to use recording mode and set up transactions that have a chance of making it somewhat deep into the auth system. This is also probably not ideal -- it would be nice to fuzz with recording turned off -- but at the moment this is sort of hard to change.soroban-fuzz-targetsturned on basically "by default": currently (on master) when you do a tests-enabled build of core, it enablesrust_stellar_core/testutilswhich turns onsoroban-fuzz-targets. This is intentional and I want to keep doing that! It's to make sure we notice if we break the fuzz targets. The fuzz targets get smoke-tested as part of normal core unit testing.fastdevmode makes a mess of this: since it's a unified build driven by cargo (not just linking separately-compiled crates), whatever version ofsoroban-env-hostthesoroban-fuzz-targetsdependency pulls in gets feature unified with the corresponding version ofsoroban-env-hostwe're using for production transaction-processing. So like if the fuzz target is pulling in p27, the production p27 getssoroban-env-host/testutilsturned on!fastdevfairly useless for development! And also scary. Nobody likes seeing divergence while working on a change.So the fix in this patch is a slight degradation in the above arrangement: we split out
rust_stellar_core/fuzz_targetsas a separate feature fromrust_stellar_core/testutils, and only turn it on when not infastdev. This way we still get smoketest coverage of the fuzz targets in the non-fastdevbuilds. We just get a warning that the fuzz target is disabled in afastdevbuild.